Skip to content

fix(live-proof): isolate exact event proof execution - #1221

Closed
vincentkoc wants to merge 7 commits into
mainfrom
fix/live-proof-three-job-isolation-20260821
Closed

fix(live-proof): isolate exact event proof execution#1221
vincentkoc wants to merge 7 commits into
mainfrom
fix/live-proof-three-job-isolation-20260821

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes an issue where an exact event review that recommends live proof would execute untrusted pull-request code in the credentialed review job, and where target-created restrictive scratch paths could make cleanup fail after valid proof was produced.

Why This Change Was Made

Event reviews now use one workflow run with three trust-separated jobs: the apply job seals an immutable exact-head core artifact, a hardcoded secretless GitHub-hosted job validates that core and produces a separately bound proof augmentation, and a fresh trusted finalizer is the sole publication, acknowledgement, and queue-completion owner. Cleanup changes no longer mutate target-created permissions.

Queued batch review remains on its existing same-job execution path and is the immediate follow-up.

User Impact

Operators keep receiving the same durable review report and optional live-proof media. Valid PASS and FAIL proof is published; a verified cleanup-only failure publishes the core review without proof; mismatched, incomplete, or failed required proof is durably requeued and leaves the finalizer red.

OpenClaw Bay Impact

Bay's durable report and comment contract is unchanged. The finalizer still publishes through the canonical record/comment path, and the exact-head core plus augmentation validation prevents Bay from observing proof from a different head or workflow run.

Documentation Impact

Updated the active docs/live-proof.md security, artifact, execution, and publication contract. Release-note context stays in this PR body because CHANGELOG.md is release-owned. The documentation names the remaining batch same-job isolation gap as an immediate follow-up.

Evidence

Current Revision

  • Source revision: 63977ae714d56929737f03d8f26b508af9c6a5c3
  • Focused workflow contract tests: 118 passed
  • TypeScript build, formatting, and lint: passed
  • git diff --check: passed
  • Full pnpm check body reached 3,654 tests: 3,637 passed. The eight local-only failures were one existing macOS tmux pane-capture race and seven target-validation fixtures blocked by a missing pnpm@10.33.0 package-mirror entry. No changed workflow contract failed.

The positive production delta implements a new security ownership boundary: immutable exact-head core and augmentation schemas, bounded inventories and digests, a secretless execution job, and a trusted sole-publisher finalizer. The old apply-job publisher/ack path and more than 700 lines of obsolete workflow assertions were removed instead of retaining an unreachable compatibility path.

Rank-up Move Disposition

The requested pre-merge exact-event Actions trace cannot exercise this branch topology: GitHub repository_dispatch always loads the repository's default-branch workflow, so a run before merge executes the old same-job design. Adding a test-only privileged dispatch route would weaken and widen this security change.

The bounded pre-merge substitute is the built workflow contract suite, exact bundle and augmentation validation tests, and the full build/lint gate above. Immediately after merge, rerun ClickClack PRs #171 and #173 exactly once each and verify the default-branch three-job trace, durable exact-head review markers, artifact publication, and no leftover scratch/worktree state.

Local test/live-proof-review-environment.test.ts reaches the existing tmux fixture but fails on this macOS host because its tmux session exits before pane capture. The isolated workflow and augmentation tests do not depend on that machine-local tmux state; hosted CI remains authoritative.

@clawsweeper

clawsweeper Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@vincentkoc

vincentkoc commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@vincentkoc

vincentkoc commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 21, 2026
@clawsweeper

clawsweeper Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 21, 2026, 4:08 PM ET / 20:08 UTC.

ClawSweeper review

What this changes

The PR splits exact-event review, untrusted live-proof execution, and trusted publication into artifact-bound GitHub Actions jobs.

Merge readiness

⛔ Blocked by patch quality or review findings - 7 items remain

This PR remains necessary because current main still runs exact live proof in the review job, but the proposed isolation is incomplete: PR-head code can recover the outer step shell’s runner variables through /proc despite env -u. The prior unsafe archive-extraction concern is addressed at the current head, but this new P1 boundary flaw blocks merge.

Priority: P1
Reviewed head: a7ff08286987f5855f5171bb18377368cfda14ec

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🌊 off-meta tidepool Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🧂 unranked krab (1/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 7 items Incomplete process boundary: The live-proof step unsets command-file variables only in a child env process; its invoking shell remains alive with those variables while untrusted target code executes.
Security claim affected: The documentation says removing the four command-file variables prevents target code from mutating later workflow state, which is not guaranteed while the parent shell remains readable by the same runner user.
Prior archive finding fixed: The finalizer now parses and validates a bounded ZIP against fixed regular-file entries before it writes the materialized augmentation.
Findings 1 actionable finding [P1] Replace the shell handoff with a real execution boundary
Security Needs attention Target execution can recover parent runner variables: The untrusted target process is a descendant of the step shell, which retains workflow command-file and runner variables even though the child environment unsets them.

How this fits together

ClawSweeper generates an exact review record, optionally executes its live-proof plan, then publishes the durable result and completes the queue lease. This PR moves proof execution to a separate job and passes only a bound proof augmentation to a fresh trusted finalizer.

flowchart LR
  A[Exact event request] --> B[Review and core bundle]
  B --> C[Live-proof job]
  C --> D[Proof augmentation]
  B --> E[Trusted finalizer]
  D --> E
  E --> F[Durable report and queue completion]
Loading

Before merge

  • Replace the shell handoff with a real execution boundary (P1) - env -u only sanitizes the child process; the step shell remains its same-user ancestor with the command-file and runner variables. PR-head code can read /proc ancestry and append to those files or recover a runner token, affecting later create/upload steps despite the claimed isolation. Replace the handoff with a boundary that removes parent access and add a regression probe.
  • Resolve security concern: Target execution can recover parent runner variables - The untrusted target process is a descendant of the step shell, which retains workflow command-file and runner variables even though the child environment unsets them.
  • Resolve merge risk (P1) - Untrusted PR code can inspect the still-running parent shell through /proc to recover command-file paths or runner credentials, then influence later live-proof-job steps.
  • Resolve merge risk (P1) - The PR body’s validation evidence names superseded revision 63977ae…, and it contains tests/checks rather than current-head real workflow proof required by repository policy.
  • Complete next step (P2) - A narrow, source-proven P1 repair can remove the parent-process exposure and add focused workflow coverage.
  • Improve patch quality - Resolve the security review concern or explain why the changed path is safe.
  • Improve patch quality - Address the highest-priority review finding and re-run the changed-surface validation.

Findings

  • [P1] Replace the shell handoff with a real execution boundary — .github/workflows/sweep.yml:1650-1655
  • [high] Target execution can recover parent runner variables — .github/workflows/sweep.yml:1650
Agent review details

Security

Needs attention: The current head safely validates the augmentation archive, but its live-proof process can still recover sensitive parent-shell state.

Review metrics

Metric Value Why it matters
Workflow scope 18 files affected; 2,476 additions and 1,422 deletions The change materially restructures the review, proof, and publication workflow.
Code-to-test delta non-test +1,257 net; tests −203 net The new artifact-boundary implementation substantially expands non-test code while consolidating older workflow assertions.

Merge-risk options

Maintainer options:

  1. Repair the target-process boundary (recommended)
    Prevent PR-head code from inheriting or recovering the parent shell’s workflow command-file and runtime environment, then add a focused regression test before merge.
  2. Pause the isolation rewrite
    Keep the PR draft until the execution model can demonstrate that untrusted code cannot affect subsequent live-proof-job steps.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Prevent PR-head execution from reading or writing workflow command files through its parent process, and add focused regression coverage.

Technical review

Best possible solution:

Create a one-way execution boundary that prevents target code from reading or writing workflow command files or influencing later actions, then preserve the safe artifact parser and update proof for the current head.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: on the Linux hosted runner, same-user PR code can walk its process ancestry through /proc and read the outer shell environment that still holds the command-file variables.

Is this the best way to solve the issue?

No. The three-job design and archive validation are sound directions, but env -u alone is not an execution boundary because it leaves the sensitive parent shell reachable.

Full review comments:

  • [P1] Replace the shell handoff with a real execution boundary — .github/workflows/sweep.yml:1650-1655
    env -u only sanitizes the child process; the step shell remains its same-user ancestor with the command-file and runner variables. PR-head code can read /proc ancestry and append to those files or recover a runner token, affecting later create/upload steps despite the claimed isolation. Replace the handoff with a boundary that removes parent access and add a regression probe.
    Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against eb8e2caabeb2.

Labels

Label changes:

  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.

Label justifications:

  • P1: The PR’s intended protection against executing untrusted pull-request code can expose runner-side workflow state.
  • merge-risk: 🚨 security-boundary: The changed job boundary governs untrusted code, runner credentials, and artifact materialization before trusted publication.
  • merge-risk: 🚨 automation: The PR restructures exact-event jobs, artifact handoff, publication, and queue completion.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

Security concerns:

  • [high] Target execution can recover parent runner variables — .github/workflows/sweep.yml:1650
    The untrusted target process is a descendant of the step shell, which retains workflow command-file and runner variables even though the child environment unsets them.
    Confidence: 0.95

Acceptance criteria:

  • [P1] pnpm run build.
  • [P1] pnpm run test:unit.
  • [P1] pnpm run check.

What I checked:

  • Incomplete process boundary: The live-proof step unsets command-file variables only in a child env process; its invoking shell remains alive with those variables while untrusted target code executes. (.github/workflows/sweep.yml:1650, a7ff08286987)
  • Security claim affected: The documentation says removing the four command-file variables prevents target code from mutating later workflow state, which is not guaranteed while the parent shell remains readable by the same runner user. (docs/live-proof.md:24, a7ff08286987)
  • Prior archive finding fixed: The finalizer now parses and validates a bounded ZIP against fixed regular-file entries before it writes the materialized augmentation. (src/live-proof/review-augmentation.ts:91, a7ff08286987)
  • Archive hardening coverage: Tests cover traversal, duplicate, symlink, hardlink-metadata, and device archive entries before materialization. (test/live-proof-review-augmentation.test.ts:281, a7ff08286987)
  • Current-main comparison: Current main still contains the prior same-job Execute exact review live proof step and has no separate live-proof/finalizer jobs, so this PR’s central change is not implemented there. (.github/workflows/sweep.yml:1450, eb8e2caabeb2)
  • Feature history: Live proof was introduced by Peter Steinberger and recently maintained by Martin Cleary; Vincent Koc also has earlier workflow history in this area. (.github/workflows/sweep.yml, 03c1f452ec49)

Likely related people:

  • Peter Steinberger: Introduced the live-proof review path and has repeatedly maintained its execution and publication flow. (role: live-proof feature introducer; confidence: high; commits: 03c1f452ec49, b85cbac03932; files: .github/workflows/sweep.yml, src/live-proof/execute.ts)
  • Martin Cleary: Recently changed exact-review live-proof setup and adjacent workflow publication behavior. (role: recent area contributor; confidence: high; commits: e869bde55a75, 378050ec37c8; files: .github/workflows/sweep.yml, src/live-proof/review-artifacts.ts)
  • Vincent Koc: Authored the current implementation and an earlier merged workflow reliability change in this area. (role: workflow contributor; confidence: high; commits: a7ff08286987, e77e387b3971; files: .github/workflows/sweep.yml, src/live-proof/review-augmentation.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-21T18:33:40.685Z sha 59bbb02 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-21T18:59:56.977Z sha 753f6db :: found issues before merge. :: [P2] Remove the release-owned changelog entry
  • reviewed 2026-08-21T19:06:42.930Z sha 63977ae :: needs changes before merge. :: [P1] Validate the augmentation archive before extracting it

@vincentkoc

vincentkoc commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 21, 2026
@vincentkoc

vincentkoc commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. and removed P2 Normal priority bug or improvement with limited blast radius. labels Aug 21, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 21, 2026
@vincentkoc vincentkoc mentioned this pull request Aug 27, 2026
Merged
@steipete

steipete commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded by #1280. Current main retired automatic live-proof generation and target execution; docs/live-proof.md now documents historical artifact publication only. The credentialed execution path this proposal sought to isolate is gone, so introducing a new three-job proof execution lane would restore machinery we deliberately removed.

Thank you for identifying the isolation problem and developing the separation. We are keeping the simpler ownership boundary rather than reviving that execution lane.

@steipete steipete closed this Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentkoc @steipete